Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement demo version of config grouping #284

Merged
merged 6 commits into from
Jan 16, 2024
Merged

Implement demo version of config grouping #284

merged 6 commits into from
Jan 16, 2024

Conversation

DavidSGK
Copy link
Member

@DavidSGK DavidSGK commented Jan 12, 2024 •
edited
Loading

Added new command lekko config group

Fully specified args example

lekko config group -n default -c config-a,config-b,config-c -o grouped-config-name -p test.config.v1beta1 -d "Description for grouped config"

Or you can just use

lekko config group

for interactive mode and to show off name suggestion

  • Generates or appends to a .proto file based on the package specified
    • Generated proto message follows the name (but camelCased) of the config name and each field will be commented with the descriptions of the original configs
  • Adds new grouped config using the generated proto type
    • Description can be specified as arg or will be something like "Grouped from blah, blah"
    • Default value will be based on default values of original configs
  • Removes old configs

Limitations:

  • AI suggestion isn't real
  • Does not handle config name/proto message name conflicts
  • Does not handle converted proto field name conflicts
  • Does not handle proto reserved keywords
  • Handles default value combining but not overrides
  • Does not handle concurrent file access

@DavidSGK DavidSGK self-assigned this Jan 12, 2024
@DavidSGK DavidSGK marked this pull request as ready for review January 16, 2024 18:09
konradjniemiec
konradjniemiec approved these changes Jan 16, 2024
View reviewed changes
cmd/lekko/feature.go
if err != nil {
return err
}
r, err := repo.NewLocal(wd, secrets.NewSecretsOrFail())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the try lib I used feels nice to avoid all this :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is definitely an egregious example of go error handling exploding haha

pkg/feature/feature.go
}

// Builder for a protobuf message definition string
type ProtoDefBuilder struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice stuff! we could also do go templating, unsure of the tradeoff between the two approaches I kinda took both in my PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah good point, I honestly didn't think about it too much - I think templating might be cleaner and less error-prone for potential edge cases so happy to switch over when we want to productionize

@DavidSGK DavidSGK merged commit f3a0da1 into main Jan 16, 2024
1 check passed
@DavidSGK DavidSGK deleted the david-config-grouping branch January 16, 2024 22:45
@DavidSGK DavidSGK mentioned this pull request Jan 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@konradjniemiec konradjniemiec konradjniemiec approved these changes

Assignees

@DavidSGK DavidSGK

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DavidSGK @konradjniemiec